-
Notifications
You must be signed in to change notification settings - Fork 370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(verifier): verify_bytes api GRO-304 #1784
Conversation
SP1 Performance Test Results Branch: yuwen/verifier-bytes-api
|
### Advanced: `verify_bytes` | ||
|
||
`sp1-verifier` also exposes [`Groth16Verifier::verify_bytes`](https://docs.rs/sp1-verifier/latest/sp1_verifier/struct.Groth16Verifier.html#method.verify_bytes) and [`PlonkVerifier::verify_bytes`](https://docs.rs/sp1-verifier/latest/sp1_verifier/struct.PlonkVerifier.html#method.verify_bytes), | ||
which verifies any Groth16 or Plonk proof from gnark. This is especially useful for verifying custom Groth16 and Plonk proofs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: Capitalize Gnark, and add a link to https://docs.gnark.consensys.io/, or the repo.
zkVM, instead of ZKVM.
} | ||
``` | ||
|
||
Public values are serialized as big-endian `Fr` values. The default gnark serialization will work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize Gnark
crates/verifier/src/groth16/mod.rs
Outdated
/// | ||
/// This method expects the raw proof bytes without the 4-byte vkey hash prefix that | ||
/// [`verify`] checks. If you have a complete proof with the prefix, use [`verify`] instead. | ||
pub fn verify_bytes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better name is verify_gnark_proof
. verify_bytes
isn't that specific. We should also add documentation to the function above to say that it's for SP1 proofs.
/// | ||
/// This method expects the raw proof bytes without the 4-byte vkey hash prefix that | ||
/// [`verify`] checks. If you have a complete proof with the prefix, use [`verify`] instead. | ||
pub fn verify_gnark_proof( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add #[must_use]
/// | ||
/// A [`Result`] containing unit `()` if the proof is valid, | ||
/// or a [`PlonkError`] if verification fails. | ||
pub fn verify_gnark_proof( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add #[must_use]
@@ -44,7 +44,7 @@ pub(crate) struct PlonkVerifyingKey { | |||
/// # Returns | |||
/// | |||
/// * `Result<bool, PlonkError>` - Returns true if the proof is valid, or an error if verification fails | |||
pub(crate) fn verify_plonk_raw( | |||
pub(crate) fn verify_plonk_algebraic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add #[must_use] so people don't forget to unwrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must_use is already on by default for Result
. doing something like this gives me a warning
crate::PlonkVerifier::verify(...);
https://succinctlabs.slack.com/archives/C04Q4J7KUBE/p1731323036949519